Clean up page header styles and remove header illustrations - #98461
Conversation
There was a problem hiding this comment.
You're right — this PR accidentally committed a stray node_modules entry (it's a committed symlink, not the actual dependencies folder). It should be dropped with:
git rm --cached node_modules
git commit -m "Remove stray node_modules symlink"
git push
baee9e5 to
de7faa5
Compare
|
@MelvinBot please mark PR as ready for review and fill out PR author checklist as best you can. |
|
🚧 shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
|
Done ✅
I intentionally left the remaining boxes unchecked because they're attestations only you can truthfully make — the cross-platform test runs (Android/iOS/mWeb/Desktop), the "no console errors" / offline / high-traffic verifications, the screenshots/videos, and the code-pattern/review-guideline confirmations. Please tick those off yourself once you've verified them and added the screenshots. Also worth noting: the stray |
|
@truph01 can you do a first pass at a review please? Keep in mind that this is a Melvin PR, so we need help with platform screenshots and code review. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
cc @dannymcclain @dubielzyk-expensify @trjExpensify @JmillsExpensify to take it for a spin |
|
Oh this feels good on desktop and mobile 😄 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
shawnborton
changed the title
|
@truph01 can you give this a final review today please? |
|
Yeah this is nice! Feels so clean. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-14.at.01.24.58.movAndroid: mWeb ChromeScreen.Recording.2026-08-14.at.01.34.24.moviOS: HybridAppScreen.Recording.2026-08-14.at.01.24.14.moviOS: mWeb SafariScreen.Recording.2026-08-14.at.01.33.20.movMacOS: Chrome / SafariScreen.Recording.2026-08-14.at.01.21.48.mov |
|
@MelvinBot can you try to fix the failing tests please? |
Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
Fixed the failing check What failed: Fix: Updated the assertion to expect the icon to be absent, mirroring the sibling narrow-layout test ( - expect(screen.getByTestId('WorkspaceCompanyCardsPageIcon')).toBeTruthy();
+ expect(screen.queryByTestId('WorkspaceCompanyCardsPageIcon')).toBeNull();All 4 tests in the suite pass locally now. Pushed as The only other failing check, |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
truph01
left a comment
truph01
left a comment
There was a problem hiding this comment.
@MelvinBot Address these comments:
| lineHeight: variables.lineHeightSizeH2, | ||
| }, | ||
|
|
||
| whiteSpaceNoWrap: { |
There was a problem hiding this comment.
whiteSpaceNoWrap duplicates existing noWrap style
styles.noWrap already exists (from the ...whiteSpace spread at line 291) with the same value { whiteSpace: 'nowrap' }. Could we reuse styles.noWrap in WorkspaceInitialPage instead of adding a new style?
There was a problem hiding this comment.
Good catch — done in fd71e51. Removed the duplicate whiteSpaceNoWrap style and reused the existing styles.noWrap in WorkspaceInitialPage:
WorkspaceInitialPage.tsx:551now usestitleStyles={styles.noWrap}.
| > | ||
| <HeaderWithBackButton | ||
| title={domainName} | ||
| titleStyles={styles.textHeadlineH2} |
There was a problem hiding this comment.
Different approach than sibling pages for H2 styling
Other domain/workspace pages use shouldUseHeadlineHeader to get the H2 title style, but this page uses titleStyles={styles.textHeadlineH2} without shouldUseHeadlineHeader. Both achieve the same result since shouldUseHeadlineHeader only controls that same style — would it be cleaner to use shouldUseHeadlineHeader here for consistency, and reserve titleStyles for the extra whiteSpaceNoWrap case in WorkspaceInitialPage?
There was a problem hiding this comment.
Agreed — switched to shouldUseHeadlineHeader for consistency with the sibling pages (DomainInitialPage.tsx:144) in fd71e51.
Confirmed this renders identically: shouldUseHeadlineHeader applies styles.textHeadlineH2 to the title (HeaderWithBackButton/index.tsx:149), and the extra styles.headerBarHeight it adds (line 232) is a no-op since styles.headerBar already sets the same height: contentHeaderHeight. I also reserved titleStyles for the noWrap extra in WorkspaceInitialPage as you suggested.
…or consistency Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
Addressed the review comments in fd71e51:
Ran |
|
@Beamanator Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
7 participants
Explanation of Change
Facelift pass focused only on page headers (a smaller chunk split out from a larger facelift PR):
HeaderWithBackButton: title now supports atitleStylesoverride, a configurablenumberOfTitleLines, and apolicyAvatarSizeso the policy/domain avatar can be sized independently.topBarLabel) now uses the h2 headline size/line-height instead offontSizeXLarge, matching theHeaderWithBackButtonscreen-header title which already usestextHeadlineH2.WorkspaceInitialPage) and Domain editor (DomainInitialPage) header titles now applytextHeadlineH2.iconillustration from theHeaderWithBackButton/WorkspacePageWithSectionsheader across Account, Workspace, and Domain pages, and removed the now-unused illustration imports/hooks.Fixed Issues
$ #98459
PROPOSAL:
Tests
Offline tests
Styling-only change; no offline-specific behavior. Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari